-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Gate the type length limit check behind a nightly flag #127670
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
imo this is the right choice - there is more fallout than I think we anticipated with re-adding the type length limit. r=me, but maybe we want to wait a day or so for anyone in the team to dissent |
@bors r=jackh726 we can easily turn this back on |
@bors rollup=never |
📣 Toolstate changed by #127670! Tested on commit 88fa119. 💔 reference on windows: test-pass → test-fail (cc @Havvy @matthewjasper @ehuss). |
Tested on commit rust-lang/rust@88fa119. Direct link to PR: <rust-lang/rust#127670> 💔 reference on windows: test-pass → test-fail (cc @Havvy @matthewjasper @ehuss). 💔 reference on linux: test-pass → test-fail (cc @Havvy @matthewjasper @ehuss).
☀️ Test successful - checks-actions |
@compiler-errors Can you please update the reference at https://github.com/rust-lang/reference/blob/master/src/attributes/limits.md#the-type_length_limit-attribute? This PR has broken the test there, and I'm not sure how the documentation should be updated. Is this attribute deprecated now? |
Sure. The attribute is not deprecated per se, but it doesn't do anything right now. Previously it didn't do anything in real code, but it was easier to make a pathological example like rust-lang/reference#1026 did after it previously broke in #72412. See #125460. I'll just remove the test. Not sure what we should do with the documentation. I'll probably just mention the attr doesn't do anything (which isn't totally true -- it actually is used in diagnostics, but I don't want to encourage people to use the attr). |
Finished benchmarking commit (88fa119): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesResults (primary -1.0%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 702.998s -> 699.561s (-0.49%) |
uniquify root goals during HIR typeck We need to rely on region identity to deal with hangs such as rust-lang/trait-system-refactor-initiative#210 and to keep the current behavior of `fn try_merge_responses`. This is a problem as borrowck starts by replacing each *occurrence* of a region with a unique inference variable. This frequently splits a single region during HIR typeck into multiple distinct regions. As we assume goals to always succeed during borrowck, relying on two occurances of a region being identical during HIR typeck causes ICE. See the now fixed examples in rust-lang/trait-system-refactor-initiative#27 and #139409. We've previously tried to avoid this issue by always *uniquifying* regions when canonicalizing goals. This prevents caching subtrees during canonicalization which resulted in hangs for very large types. People rely on such types in practice, which caused us to revert our attempt to reinstate `#[type_length_limit]` in #127670. The complete list of changes here: - #107981 - #110180 - #114117 - #130821 After more consideration, all occurrences of such large types need to happen outside of typeck/borrowck. We know this as we already walk over all types in the MIR body when replacing their regions with nll vars. This PR therefore enables us to rely on region identity inside of the trait solver by exclusively **uniquifying root goals during HIR typeck**. These are the only goals we assume to hold during borrowck. This is insufficient as type inference variables may "hide" regions we later uniquify. Because of this, we now stash proven goals which depend on inference variables in HIR typeck and reprove them after writeback. This closes rust-lang/trait-system-refactor-initiative#127. This was originally part of #144258 but I've moved it into a separate PR. While I believe we need to rely on region identity to fix the performance issues in some way, I don't know whether #144258 is the best approach to actually do so. Regardless of how we deal with the hangs however, this change is necessary and desirable regardless. r? `@compiler-errors` or `@BoxyUwU`
uniquify root goals during HIR typeck We need to rely on region identity to deal with hangs such as rust-lang/trait-system-refactor-initiative#210 and to keep the current behavior of `fn try_merge_responses`. This is a problem as borrowck starts by replacing each *occurrence* of a region with a unique inference variable. This frequently splits a single region during HIR typeck into multiple distinct regions. As we assume goals to always succeed during borrowck, relying on two occurances of a region being identical during HIR typeck causes ICE. See the now fixed examples in rust-lang/trait-system-refactor-initiative#27 and rust-lang/rust#139409. We've previously tried to avoid this issue by always *uniquifying* regions when canonicalizing goals. This prevents caching subtrees during canonicalization which resulted in hangs for very large types. People rely on such types in practice, which caused us to revert our attempt to reinstate `#[type_length_limit]` in rust-lang/rust#127670. The complete list of changes here: - rust-lang/rust#107981 - rust-lang/rust#110180 - rust-lang/rust#114117 - rust-lang/rust#130821 After more consideration, all occurrences of such large types need to happen outside of typeck/borrowck. We know this as we already walk over all types in the MIR body when replacing their regions with nll vars. This PR therefore enables us to rely on region identity inside of the trait solver by exclusively **uniquifying root goals during HIR typeck**. These are the only goals we assume to hold during borrowck. This is insufficient as type inference variables may "hide" regions we later uniquify. Because of this, we now stash proven goals which depend on inference variables in HIR typeck and reprove them after writeback. This closes rust-lang/trait-system-refactor-initiative#127. This was originally part of rust-lang/rust#144258 but I've moved it into a separate PR. While I believe we need to rely on region identity to fix the performance issues in some way, I don't know whether rust-lang/rust#144258 is the best approach to actually do so. Regardless of how we deal with the hangs however, this change is necessary and desirable regardless. r? `@compiler-errors` or `@BoxyUwU`
uniquify root goals during HIR typeck We need to rely on region identity to deal with hangs such as rust-lang/trait-system-refactor-initiative#210 and to keep the current behavior of `fn try_merge_responses`. This is a problem as borrowck starts by replacing each *occurrence* of a region with a unique inference variable. This frequently splits a single region during HIR typeck into multiple distinct regions. As we assume goals to always succeed during borrowck, relying on two occurances of a region being identical during HIR typeck causes ICE. See the now fixed examples in rust-lang/trait-system-refactor-initiative#27 and rust-lang/rust#139409. We've previously tried to avoid this issue by always *uniquifying* regions when canonicalizing goals. This prevents caching subtrees during canonicalization which resulted in hangs for very large types. People rely on such types in practice, which caused us to revert our attempt to reinstate `#[type_length_limit]` in rust-lang/rust#127670. The complete list of changes here: - rust-lang/rust#107981 - rust-lang/rust#110180 - rust-lang/rust#114117 - rust-lang/rust#130821 After more consideration, all occurrences of such large types need to happen outside of typeck/borrowck. We know this as we already walk over all types in the MIR body when replacing their regions with nll vars. This PR therefore enables us to rely on region identity inside of the trait solver by exclusively **uniquifying root goals during HIR typeck**. These are the only goals we assume to hold during borrowck. This is insufficient as type inference variables may "hide" regions we later uniquify. Because of this, we now stash proven goals which depend on inference variables in HIR typeck and reprove them after writeback. This closes rust-lang/trait-system-refactor-initiative#127. This was originally part of rust-lang/rust#144258 but I've moved it into a separate PR. While I believe we need to rely on region identity to fix the performance issues in some way, I don't know whether rust-lang/rust#144258 is the best approach to actually do so. Regardless of how we deal with the hangs however, this change is necessary and desirable regardless. r? `@compiler-errors` or `@BoxyUwU`
uniquify root goals during HIR typeck We need to rely on region identity to deal with hangs such as rust-lang/trait-system-refactor-initiative#210 and to keep the current behavior of `fn try_merge_responses`. This is a problem as borrowck starts by replacing each *occurrence* of a region with a unique inference variable. This frequently splits a single region during HIR typeck into multiple distinct regions. As we assume goals to always succeed during borrowck, relying on two occurances of a region being identical during HIR typeck causes ICE. See the now fixed examples in rust-lang/trait-system-refactor-initiative#27 and rust-lang/rust#139409. We've previously tried to avoid this issue by always *uniquifying* regions when canonicalizing goals. This prevents caching subtrees during canonicalization which resulted in hangs for very large types. People rely on such types in practice, which caused us to revert our attempt to reinstate `#[type_length_limit]` in rust-lang/rust#127670. The complete list of changes here: - rust-lang/rust#107981 - rust-lang/rust#110180 - rust-lang/rust#114117 - rust-lang/rust#130821 After more consideration, all occurrences of such large types need to happen outside of typeck/borrowck. We know this as we already walk over all types in the MIR body when replacing their regions with nll vars. This PR therefore enables us to rely on region identity inside of the trait solver by exclusively **uniquifying root goals during HIR typeck**. These are the only goals we assume to hold during borrowck. This is insufficient as type inference variables may "hide" regions we later uniquify. Because of this, we now stash proven goals which depend on inference variables in HIR typeck and reprove them after writeback. This closes rust-lang/trait-system-refactor-initiative#127. This was originally part of rust-lang/rust#144258 but I've moved it into a separate PR. While I believe we need to rely on region identity to fix the performance issues in some way, I don't know whether rust-lang/rust#144258 is the best approach to actually do so. Regardless of how we deal with the hangs however, this change is necessary and desirable regardless. r? `@compiler-errors` or `@BoxyUwU`
uniquify root goals during HIR typeck We need to rely on region identity to deal with hangs such as rust-lang/trait-system-refactor-initiative#210 and to keep the current behavior of `fn try_merge_responses`. This is a problem as borrowck starts by replacing each *occurrence* of a region with a unique inference variable. This frequently splits a single region during HIR typeck into multiple distinct regions. As we assume goals to always succeed during borrowck, relying on two occurances of a region being identical during HIR typeck causes ICE. See the now fixed examples in rust-lang/trait-system-refactor-initiative#27 and rust-lang/rust#139409. We've previously tried to avoid this issue by always *uniquifying* regions when canonicalizing goals. This prevents caching subtrees during canonicalization which resulted in hangs for very large types. People rely on such types in practice, which caused us to revert our attempt to reinstate `#[type_length_limit]` in rust-lang/rust#127670. The complete list of changes here: - rust-lang/rust#107981 - rust-lang/rust#110180 - rust-lang/rust#114117 - rust-lang/rust#130821 After more consideration, all occurrences of such large types need to happen outside of typeck/borrowck. We know this as we already walk over all types in the MIR body when replacing their regions with nll vars. This PR therefore enables us to rely on region identity inside of the trait solver by exclusively **uniquifying root goals during HIR typeck**. These are the only goals we assume to hold during borrowck. This is insufficient as type inference variables may "hide" regions we later uniquify. Because of this, we now stash proven goals which depend on inference variables in HIR typeck and reprove them after writeback. This closes rust-lang/trait-system-refactor-initiative#127. This was originally part of rust-lang/rust#144258 but I've moved it into a separate PR. While I believe we need to rely on region identity to fix the performance issues in some way, I don't know whether rust-lang/rust#144258 is the best approach to actually do so. Regardless of how we deal with the hangs however, this change is necessary and desirable regardless. r? `@compiler-errors` or `@BoxyUwU`
uniquify root goals during HIR typeck We need to rely on region identity to deal with hangs such as rust-lang/trait-system-refactor-initiative#210 and to keep the current behavior of `fn try_merge_responses`. This is a problem as borrowck starts by replacing each *occurrence* of a region with a unique inference variable. This frequently splits a single region during HIR typeck into multiple distinct regions. As we assume goals to always succeed during borrowck, relying on two occurances of a region being identical during HIR typeck causes ICE. See the now fixed examples in rust-lang/trait-system-refactor-initiative#27 and rust-lang/rust#139409. We've previously tried to avoid this issue by always *uniquifying* regions when canonicalizing goals. This prevents caching subtrees during canonicalization which resulted in hangs for very large types. People rely on such types in practice, which caused us to revert our attempt to reinstate `#[type_length_limit]` in rust-lang/rust#127670. The complete list of changes here: - rust-lang/rust#107981 - rust-lang/rust#110180 - rust-lang/rust#114117 - rust-lang/rust#130821 After more consideration, all occurrences of such large types need to happen outside of typeck/borrowck. We know this as we already walk over all types in the MIR body when replacing their regions with nll vars. This PR therefore enables us to rely on region identity inside of the trait solver by exclusively **uniquifying root goals during HIR typeck**. These are the only goals we assume to hold during borrowck. This is insufficient as type inference variables may "hide" regions we later uniquify. Because of this, we now stash proven goals which depend on inference variables in HIR typeck and reprove them after writeback. This closes rust-lang/trait-system-refactor-initiative#127. This was originally part of rust-lang/rust#144258 but I've moved it into a separate PR. While I believe we need to rely on region identity to fix the performance issues in some way, I don't know whether rust-lang/rust#144258 is the best approach to actually do so. Regardless of how we deal with the hangs however, this change is necessary and desirable regardless. r? `@compiler-errors` or `@BoxyUwU`
uniquify root goals during HIR typeck We need to rely on region identity to deal with hangs such as rust-lang/trait-system-refactor-initiative#210 and to keep the current behavior of `fn try_merge_responses`. This is a problem as borrowck starts by replacing each *occurrence* of a region with a unique inference variable. This frequently splits a single region during HIR typeck into multiple distinct regions. As we assume goals to always succeed during borrowck, relying on two occurances of a region being identical during HIR typeck causes ICE. See the now fixed examples in rust-lang/trait-system-refactor-initiative#27 and rust-lang/rust#139409. We've previously tried to avoid this issue by always *uniquifying* regions when canonicalizing goals. This prevents caching subtrees during canonicalization which resulted in hangs for very large types. People rely on such types in practice, which caused us to revert our attempt to reinstate `#[type_length_limit]` in rust-lang/rust#127670. The complete list of changes here: - rust-lang/rust#107981 - rust-lang/rust#110180 - rust-lang/rust#114117 - rust-lang/rust#130821 After more consideration, all occurrences of such large types need to happen outside of typeck/borrowck. We know this as we already walk over all types in the MIR body when replacing their regions with nll vars. This PR therefore enables us to rely on region identity inside of the trait solver by exclusively **uniquifying root goals during HIR typeck**. These are the only goals we assume to hold during borrowck. This is insufficient as type inference variables may "hide" regions we later uniquify. Because of this, we now stash proven goals which depend on inference variables in HIR typeck and reprove them after writeback. This closes rust-lang/trait-system-refactor-initiative#127. This was originally part of rust-lang/rust#144258 but I've moved it into a separate PR. While I believe we need to rely on region identity to fix the performance issues in some way, I don't know whether rust-lang/rust#144258 is the best approach to actually do so. Regardless of how we deal with the hangs however, this change is necessary and desirable regardless. r? `@compiler-errors` or `@BoxyUwU`
Effectively disables the type length limit by introducing a
-Zenforce-type-length-limit
which defaults tofalse
, since making the length limit actually be enforced ended up having a worse fallout than expected. We still keep the code around, but the type length limit attr is now a noop (except for its usage in some diagnostics code?).r? @lcnr -- up to you to decide what team consensus we need here since this reverses an FCP decision.
Reopens #125460 (if we decide to reopen it or keep it closed)
Effectively reverses the decision FCP'd in #125507
Closes #127346